Skip to content

gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded#153365

Open
maurycy wants to merge 20 commits into
python:mainfrom
maurycy:tachyon-parse_async_frame_chain-limit
Open

gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded#153365
maurycy wants to merge 20 commits into
python:mainfrom
maurycy:tachyon-parse_async_frame_chain-limit

Conversation

@maurycy

@maurycy maurycy commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The PR hardens and cleans up frame, coro and task-waiter walks a bit, by making them iterative and bounded.

I'm addressing two issues here:

  1. No walk limits. The walks use pointers read from the target without any guarantees, so a torn read could stitch a cycle. Not to mention adversial targets. process_frame_chain() already had 1024 + 512 limit, but the PR extracts it as MAX_FRAME_CHAIN_DEPTH and applies in the parse_coro_chain(), used by both sync and async paths, and parse_async_frame_chain(). The task-waiter walk in get_async_stack_trace() gets a separate limit (MAX_TASK_WAITER_WALK_TASKS), on the total number of tasks visited (including duplicates), since its' a graph, not a chain.
  2. Stack overflows. Both coro and task-waiter walks were recursive. With 4KB (SIZEOF_TASK_OBJ) 256 was enough to overflow a 1MiB stack.

The obvious context here is avoiding infinite loops. Truth be told, I think that in some places torn reads were also responsible for avoiding them and exists. :-)

Comment thread Modules/_remote_debugging/asyncio.c
#define MAX_STACK_CHUNK_SIZE (16 * 1024 * 1024) /* 16 MB max for stack chunks */
#define MAX_LONG_DIGITS 64 /* Allows values up to ~2^1920 */
#define MAX_SET_TABLE_SIZE (1 << 20) /* 1 million entries max for set iteration */
#define MAX_FRAME_CHAIN_DEPTH (1024 + 512) /* Iteration bound for frame chain walks */

@maurycy maurycy Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the scope but maybe MAX_THREADS, MAX_STACK_CHUNKS, MAX_LINETABLE_ENTRIES or MAX_ITERATIONS are worth keeping here too

@maurycy maurycy changed the title gh-153364: Add frame limit in get_async_stack_trace() and parse_coro_chain() gh-153364: Limit frame, coroutine, and task-waiter chain walks Jul 10, 2026
Comment thread Modules/_remote_debugging/asyncio.c Outdated
Comment thread Lib/test/test_external_inspection.py Outdated
Comment thread Modules/_remote_debugging/_remote_debugging.h Outdated
Comment thread Modules/_remote_debugging/asyncio.c Outdated
Comment thread Modules/_remote_debugging/asyncio.c Outdated
PyObject *result,
size_t depth
) {
if (depth >= MAX_TASK_WAITER_CHAIN_DEPTH) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can still overflow a 1 MiB stack: every recursive level keeps the 4 KiB task_obj alive, so 256 levels exhaust the stack before this check fires. We should walk the waiter graph iteratively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablogsal I've changed both to iterative.

@bedevere-app

bedevere-app Bot commented Jul 11, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@maurycy

maurycy commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-app

bedevere-app Bot commented Jul 13, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

@bedevere-app bedevere-app Bot requested a review from pablogsal July 13, 2026 07:50
@maurycy maurycy changed the title gh-153364: Limit frame, coroutine, and task-waiter chain walks gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded Jul 13, 2026
return -1;
}

if (ref_cnt) {

@maurycy maurycy Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was a bug:

ref 1885988

PyObject *task_info = PyList_GET_ITEM(result, i);
PyObject *waiters = PyStructSequence_GET_ITEM(task_info, 3);
for (Py_ssize_t j = 0; j < PyList_GET_SIZE(waiters); j++) {
if (PyList_GET_SIZE(result) >= MAX_TASK_WAITER_WALK_TASKS) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not keeping the visited set, so duplicated are counted towards. I believe that 65536 gives enough head room?

int
process_task_and_waiters(
static int
process_task_waiters(

@maurycy maurycy Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BFS. Leveraging results as the queue. Note that the current order is not documented and not relied by tests.

void *context
);

extern int process_single_task_node(

@maurycy maurycy Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used only in the asyncio.c. Does it, and friends, need to be in the header?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants